pkg/utils/otel.go (2)

95-98: Minor formatting and potential concern with handleErr pattern.

There's a missing space after the comma on line 97. Additionally, this handleErr function is defined but only used when newTraceProvider fails. The pattern is correct for cleanup, but ensure the named return err is intended to be set via closure side-effect.

🔧 Suggested formatting fix

 	handleErr := func(inErr error) {
-		err = errors.Join(inErr,shutdown(ctx))
+		err = errors.Join(inErr, shutdown(ctx))
 	}

---

128-144: Consider validating the protocol value.

Unknown protocol values (anything other than "http") silently default to gRPC. This could cause confusion if a user misconfigures OTEL_EXPORTER_OTLP_PROTOCOL with an invalid value like "https" or a typo.

Consider either:

1. Logging when falling back to gRPC
2. Validating and returning an error for unsupported protocols

♻️ Option: Add explicit protocol validation

 func newTraceProvider(ctx context.Context, config OTelConfig) (*trace.TracerProvider, error) {
 	var traceClient otlptrace.Client
 
-	if config.Protocol == "http" {
+	switch config.Protocol {
+	case "http":
 		opts := []otlptracehttp.Option{
 			otlptracehttp.WithEndpoint(config.Endpoint),
 		}
 		if config.InsecureExporter {
 			opts = append(opts, otlptracehttp.WithInsecure())
 		}
 		traceClient = otlptracehttp.NewClient(opts...)
-	} else {
+	case "grpc", "":
 		opts := []otlptracegrpc.Option{
 			otlptracegrpc.WithEndpoint(config.Endpoint),
 		}
 		if config.InsecureExporter {
 			opts = append(opts, otlptracegrpc.WithInsecure())
 		}
 		traceClient = otlptracegrpc.NewClient(opts...)
+	default:
+		return nil, fmt.Errorf("unsupported OTLP protocol: %s (expected 'grpc' or 'http')", config.Protocol)
 	}

pkg/utils/otel_test.go (2)

62-69: Consider using t.Setenv for automatic cleanup.

The current approach of manually calling os.Unsetenv and os.Setenv doesn't restore the original environment after the test completes. Using t.Setenv (available since Go 1.17) automatically restores the original value when the test completes, providing better isolation.

♻️ Suggested improvement

 		t.Run(tt.name, func(t *testing.T) {
-			// Clear relevant env vars
-			envKeys := []string{
-				"OTEL_SERVICE_NAME",
-				"OTEL_SERVICE_VERSION",
-				"OTEL_EXPORTER_OTLP_ENDPOINT",
-				"OTEL_EXPORTER_OTLP_PROTOCOL",
-				"OTEL_TRACES_EXPORTER",
-				"OTEL_EXPORTER_OTLP_INSECURE",
-			}
-			for _, key := range envKeys {
-				os.Unsetenv(key)
-			}
-
-			// Set test env vars
+			// Set test env vars (t.Setenv auto-restores after test)
 			for key, value := range tt.envVars {
-				os.Setenv(key, value)
+				t.Setenv(key, value)
 			}

Note: For default values test case, you may need to explicitly set empty values or use a helper to ensure vars are unset.

---

117-135: Test for SetupOTelSDK should ensure complete environment isolation.

This test only unsets OTEL_TRACES_EXPORTER but other OTEL environment variables from previous tests or the system environment could still affect the test. Consider clearing all relevant environment variables for consistent test behavior.

♻️ Suggested improvement

 func TestSetupOTelSDK(t *testing.T) {
-	// Clear env vars to use defaults
-	os.Unsetenv("OTEL_TRACES_EXPORTER")
+	// Clear all OTEL env vars to ensure defaults are used
+	for _, key := range []string{
+		"OTEL_SERVICE_NAME",
+		"OTEL_SERVICE_VERSION",
+		"OTEL_EXPORTER_OTLP_ENDPOINT",
+		"OTEL_EXPORTER_OTLP_PROTOCOL",
+		"OTEL_TRACES_EXPORTER",
+		"OTEL_EXPORTER_OTLP_INSECURE",
+	} {
+		t.Setenv(key, "")
+	}

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

charts/lfx-v2-access-check/values.yaml (1)

54-73: otel.insecure is a string; Helm if treats "false" as true—align value type with intended rendering.

Right now insecure: "false" will still satisfy if .Values.otel.insecure (non-empty), so the deployment will emit OTEL_EXPORTER_OTLP_INSECURE even when “false”. If you intended “only emit when true”, make it a boolean.

Proposed patch (use boolean in values)

-  insecure: "false"
+  insecure: false

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

pkg/utils/otel.go (1)

44-80: LGTM!

The OTelConfig struct is well-documented with clear environment variable mappings. The Protocol field validation concern has been previously raised.

pkg/logging/context.go (1)

15-37: LGTM! Useful helpers for manual trace context injection.

These complement the automatic OtelHandler integration in pkg/log/log.go by providing explicit control for ad-hoc logger enrichment scenarios.

💡 Optional: Reduce duplication between the two functions

 func LogAttrsFromContext(ctx context.Context) []slog.Attr {
 	spanCtx := trace.SpanContextFromContext(ctx)
 	if !spanCtx.IsValid() {
 		return nil
 	}
 	return []slog.Attr{
 		slog.String("trace_id", spanCtx.TraceID().String()),
 		slog.String("span_id", spanCtx.SpanID().String()),
 	}
 }
 
 func LogWithContext(ctx context.Context, logger *slog.Logger) *slog.Logger {
-	spanCtx := trace.SpanContextFromContext(ctx)
-	if !spanCtx.IsValid() {
+	attrs := LogAttrsFromContext(ctx)
+	if attrs == nil {
 		return logger
 	}
-	return logger.With(
-		slog.String("trace_id", spanCtx.TraceID().String()),
-		slog.String("span_id", spanCtx.SpanID().String()),
-	)
+	args := make([]any, len(attrs))
+	for i, attr := range attrs {
+		args[i] = attr
+	}
+	return logger.With(args...)
 }

cmd/lfx-access-check/main.go (1)

28-40: Consider adding a timeout to the OTel shutdown context.

The OTel initialization and error handling are correct. Using context.Background() for shutdown is reasonable since the main context may already be cancelled. However, adding a timeout ensures the shutdown doesn't hang indefinitely if the OTLP endpoint is unresponsive.

⏱️ Suggested timeout for graceful shutdown

 defer func() {
-	if shutdownErr := otelShutdown(context.Background()); shutdownErr != nil {
+	shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer shutdownCancel()
+	if shutdownErr := otelShutdown(shutdownCtx); shutdownErr != nil {
 		slog.Error("error shutting down OpenTelemetry SDK", "error", shutdownErr)
 	}
 }()

You'll need to add "time" to the imports.

pkg/utils/otel.go (1)

356-390: LGTM!

Logger provider setup is consistent with the other providers.

Optional: The exporter configuration logic (endpoint and insecure options) is duplicated across newTraceProvider, newMetricsProvider, and newLoggerProvider. Consider extracting a helper if this pattern expands, but the current explicit approach is clear and maintainable.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}